Conversation
kirk-baird
left a comment
kirk-baird
left a comment
There was a problem hiding this comment.
Looks good to me.
Just 2 nit picks which you can take or leave, just a bit of a coding style.
Co-authored-by: Kirk Baird <kirk@sigmaprime.io>
There was a problem hiding this comment.
Pull Request Overview
This PR addresses security and clarity issues in the BLST‑based lagrange interpolation implementation, following an internal audit. Key changes include correcting the use of blst’s scalar conversion function, renaming variables to better reflect their roles, and adding explicit checks for zero keys and invalid thresholds.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| anchor/common/bls_lagrange/src/lib.rs | Added a new error variant and updated tests to cover zero key and signature issues. |
| anchor/common/bls_lagrange/src/blst.rs | Switched to using blst_scalar_from_le_bytes and improved scratch memory initialization. |
| anchor/common/bls_lagrange/src/blsful.rs | Revised secret key extraction and error handling for signatures and secrets. |
| anchor/common/bls_lagrange/Cargo.toml | Changed the default feature from blsful to blst_single_thread. |
Comments suppressed due to low confidence (1)
anchor/common/bls_lagrange/src/blsful.rs:60
- [nitpick] Consider replacing the numeric check with a clear boolean check (e.g., if scalar.is_zero() { ... }) to improve readability and reduce potential confusion around the unwrap_u8() call.
if scalar.is_zero().unwrap_u8() != 0 {
Co-authored-by: diegomrsantos <diegomrsantos@gmail.com>
Co-authored-by: diegomrsantos <diegomrsantos@gmail.com>
|
@jking-aus I think you should take a look at this, especially as it sets |
jking-aus
left a comment
jking-aus
left a comment
There was a problem hiding this comment.
Thanks for the audit kirk! And Daniel, must feel good to get that warning out of codebase.
Issue Addressed
After an internal audit, one major and several minor issues were found in our lagrange interpolation implementation using
blst. Recall that this implementation was not in active use until now.Proposed Changes
I will go over the issues found and the solution implemented for it.
Misuse of
blst_scalar_from_uint64🌶️blst_scalar_from_uint64reads fouru64instead of one (as assumed by the author). This caused theFromandTryFromimplementations to read additional memory after the passedu64. This was not caught before as the crate was never used productively, and the tests triggered this in a way where the IDs ended up the same regardless, causing no apparent issue.We now instead use
blst_scalar_from_le_bytes, which allows us to explicitly choose the amount of bytes to read. Fixed in commit 25bb849.Rename
keysandmskThese variable names are unclear: while "keys" corresponds to the type, they are not actually used as keys, but coefficients in our polynomial.
mskis an unclear acronym.In commit ab74ae0,
keyshas been renamed torandom_coefficients, andmskhas been renamed tocoefficients.Check for zero input key
The coefficients must not be zero. This includes the secret as constant coefficient, and the random coefficients.
The random coefficients can not be zero, as
blstskey_genensures that it does not generate a zero key. While it difficult for a user to even create a zero key to be split, a check was added in commit 3b72be2 to be safe.Initialise scratch memory
We create some scratch space memory for use in
blsts implementation of Pippenger's algorithm. While the memory is allowed to be uninitialised as a raw pointer is passed, we switch to initialising theVecinstead of just allocating it in commit 42ba302 for clarity.Additional Info
Now that we are audited, this PR switches to use the single thread
blstimplementation by default for now, and remove the warning. Some additional error case tests have been added.